-
Notifications
You must be signed in to change notification settings - Fork 13
Switch to CPack for packaging #379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Switch to CPack for packaging #379
Conversation
d1d9612
to
552f8f1
Compare
Windows building fails with: ``` error[E0308]: mismatched types --> src\cass_error.rs:137:81 | 137 | CassError((CassErrorSource::CASS_ERROR_SOURCE_SERVER.0 << 24) | (*num as u32)) | ^^^^^^^^^^^^^ expected `i32`, found `u32` error[E0277]: no implementation for `i32 | u32` --> src\cass_error.rs:137:79 | 137 | CassError((CassErrorSource::CASS_ERROR_SOURCE_SERVER.0 << 24) | (*num as u32)) | ^ no implementation for `i32 | u32` ```
2802f9a
to
81c2641
Compare
81c2641
to
4c2a4dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the approach looks correct. Not knowing CPack, I'm not sure though.
set(CPACK_PACKAGE_CONTACT "ScyllaDB <info@scylladb.com>") | ||
set(CPACK_PACKAGE_DESCRIPTION_SUMMARY | ||
"ScyllaDB C++ driver backed by the Rust core driver") | ||
set(CPACK_PACKAGE_HOMEPAGE_URL "https://github.com/scylladb/cpp-rust-driver") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set(CPACK_PACKAGE_HOMEPAGE_URL "https://github.com/scylladb/cpp-rust-driver") | |
set(CPACK_PACKAGE_HOMEPAGE_URL "https://github.com/scylladb/cpp-rs-driver") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
set(CPACK_GENERATOR "productbuild;DragNDrop") | ||
set(CPACK_PACKAGE_FILE_NAME | ||
"${_CPACK_PACKAGE_NAME}-${PROJECT_VERSION_STRING}-macos") | ||
set(CPACK_PRODUCTBUILD_IDENTIFIER "com.scylladb.cpp-rust-driver") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Sometimes you write cpp-driver
and sometimes cpp-rust-driver
. Is the discrepancy intended?
🔧 cpp-rust-driver
-> cpp-rs-driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
11. Build binary packages with CPack on each target platform (Linux, macOS, Windows). The README provides the exact command sequence (`cmake -S`, `cmake --build`, `cpack -G <generator>`). If you do not have access to the platforms locally, trigger a run of the reusable workflow `.github/workflows/build-cpack-packages.yml` from another workflow and download the resulting artifacts. | ||
12. Go to https://github.com/scylladb/cpp-rust-driver/releases , click the `Draft new release` button and follow the procedure to create a new release on GitHub. Use the release notes as its description. | ||
13. After the release is published, the `Attach Packages to Release` workflow (`.github/workflows/release-upload-packages.yml`) automatically builds fresh packages using CPack and uploads every artifact to the release. Verify that the workflow finished successfully and that RPM/DEB/MSI/PKG/DMG files are attached. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ What is the intended difference between building packages in point 11 and 13?
Debian packages set shlibdeps automatically; RPM packages target the | ||
`Applications/Databases` group and use release number `1` by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ What does it mean? It's all Greek to me.
Both outputs bundle the compiled libraries alongside headers and pkg-config | ||
manifests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ This also uses naming that is unknown to me.
// On windows enums are i32, so we need to cast it | ||
#[allow(clippy::unnecessary_cast)] | ||
let source = CassErrorSource::CASS_ERROR_SOURCE_SERVER.0 as u32; | ||
let code = (source << 24) | (*num as u32); | ||
CassError(code as _) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite an unexpected catch.
workflow_call: | ||
inputs: | ||
build-type: | ||
description: CMake build type used for packaging | ||
type: string | ||
default: Release | ||
extra-cmake-flags: | ||
description: Additional flags passed to CMake configure step | ||
type: string | ||
default: "" | ||
save-artifacts: | ||
description: Save built packages as artifacts | ||
type: boolean | ||
default: false | ||
secrets: {} | ||
workflow_dispatch: | ||
inputs: | ||
build-type: | ||
description: CMake build type used for packaging | ||
type: string | ||
default: Release | ||
extra-cmake-flags: | ||
description: Additional flags passed to CMake configure step | ||
type: string | ||
default: "" | ||
save-artifacts: | ||
description: Save built packages as artifacts | ||
type: boolean | ||
default: false | ||
secrets: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 It's a pity we need to be that WET instead of DRY. Chat GPT, however, hasn't come up with a better solution for enabling both manual dispatch and being called from another workflow, both with the same parameters and configuration.
SHELL = bash | ||
|
||
SHELL = bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why the duplication?
SHELL = bash | ||
ifeq ($(OS),Windows_NT) | ||
SHELL := pwsh.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 I believe it's an error to mix eager binding :=
operator with late binding operator =
for the same variable. Do you agree?
sudo dpkg -i "${DRIVER_PACKAGES[@]}" | ||
sudo apt-get install -f -y | ||
- name: Build smoke-test application package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Let's add a missing (DEB and RPM)
suffix, so this step's name isconsistency with other steps ((DEB)
only) is explicit.
Pre-review checklist
Makefile
in{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER
.Fixes:
annotations to PR description.